Zero-fee commitments support#660
Conversation
|
👋 Thanks for assigning @tnull as a reviewer! |
| /// `option_anchor_zero_fee_commitments`. All the caveats and warnings in | ||
| /// [`AnchorChannelsConfig`] still apply. | ||
| /// [`AnchorChannelsConfig`]: Config::anchor_channels_config | ||
| pub enable_zero_fee_commitments: bool, |
There was a problem hiding this comment.
I don't think we'll wan to add a new flag here that's probably hard to understand for the user? Rather, shouldn't we enable this for the user based on our current 'trust model settings' here?
Also, from these docs it's very unclear what this setting even does, when the user would want to enable it, what drawbacks it has, etc
There was a problem hiding this comment.
FWIW, thinking about it again it seems that we should never set negotiate_anchor_zero_fee_commitments until we're positive our chain sources support submitpackage/TRUC, no? And once we are positive, we would always set it?
There was a problem hiding this comment.
Rather, shouldn't we enable this for the user based on our current 'trust model settings' here?
Don't quite follow here could you expand ? I think 0FC channels merit an explicit setting somewhere rather than derived from trust model settings.
Also, from these docs it's very unclear what this setting even does, when the user would want to enable it, what drawbacks it has, etc
Yes will expand
There was a problem hiding this comment.
Don't quite follow here could you expand ? I think 0FC channels merit an explicit setting somewhere rather than derived from trust model settings.
Why, what do they fundamentally change for the user compared to our three current modes (fully trusted/keep 0-reserve, still try to claim/keep X reserve, try to claim)? Keep in mind that communicating these three modes to the user is already very hard, they always have a very hard time understanding what this means. Now, how would we communicate any changed assumptions for 0FC here? If we already trust our counterparty already, wouldn't we always want to enable 0FC for the UX improvements?
There was a problem hiding this comment.
Why, what do they fundamentally change for the user compared to our three current modes (fully trusted/keep 0-reserve, still try to claim/keep X reserve, try to claim)?
Let me see I don't think they change anything ? Whether to enable or disable 0FC channels is orthogonal to these modes ie trusted_peers_no_reserve and per_channel_reserve_sats should have no influence on whether we enable 0FC channels (only that per_channel_reserve_sats should be set to some value). I suspect you don't agree :)
If we already trust our counterparty already, wouldn't we always want to enable 0FC for the UX improvements?
It seems to me trusting our counterparty -> keeping 0 reserve is orthogonal to whether the user wants to enable 0FC channels ? for example a user trusts their counterparty, but wants to wait for greater adoption of Core v29+ before using 0FC channels.
cb1cdf9 to
c874049
Compare
|
Marked as draft: I think we should wait for electrum and esplora submit package support before merging this PR. |
c874049 to
ef3ba7a
Compare
|
Successfully opened some 0FC channels, made payments, and force closed them with the esplora diff in this branch. https://mutinynet.com/tx/508a954d85f5b7daf224a2fdc54ea6de9a26c0f62f7d58284bf61c3cdfd346e6 |
ef3ba7a to
3ebd017
Compare
AnchorChannelsConfig::enable_zero_fee_commitments3ebd017 to
eda13d4
Compare
e136f33 to
d4a2a04
Compare
771f45b to
a7c7911
Compare
When we are preparing to open a channel to a peer, we should reserve onchain funds for an anchor channel when the peer's init features signals anchor channels as optional, as channel negotiation with such a peer can result in an anchor channel. Tests written with codex.
We previously allowed users to disable anchor channels and drain their anchor reserve while still having anchor channels open or pending resolution. This was acceptable for keyed anchor channels, as the commitment transaction therein still contained some fees, and had some chance of getting mined into a block without any anchor bumps. In upcoming commits, we will add support for 0FC channels, and their commitment transactions have zero fees and depend entirely on the anchor reserve to reach miners and get confirmed in a block. It is thus dangerous to disable anchor channels and drain the reserve after 0FC channels have been opened. Therefore, we make `AnchorChannelsConfig` required, and prevent this case from ever happening.
The patch adds support for the `broadcast_package` method added in electrum protocol v1.6. Upcoming commits will require this patch to pass CI.
The mempool/electrs docker image used in those tests only supports submitpackage via the esplora interface, not the electrum interface.
We bump the Bitcoin Core version used in kotlin and python tests to support ephemeral dust. This is required for 0FC channels.
In upcoming commits we will read this knob to determine whether to negotiate 0FC channels. For now, we make a best-effort attempt to make sure the configured chain source supports 0FC channels if this knob is set. Do this roundtrip at the same time we make a roundtrip to retrieve the feerates to keep startup as fast as possible.
Implementations of `BroadcasterInterface` cannot assume any topological ordering on the transactions received, so here we order the received transactions before adding them to the broadcast queue. Any consumers of the queue can now assume all transactions received to be topologically sorted. Codex wrote the tests.
These will be useful when we add support for broadcasting packages in an upcoming commit.
We rely on the `BroadcasterInterface` contract whereby any multi-transaction vector must be a single child and its parents. In a prior commit, we added the guarantee that any packages received from the broadcast queue are already topologically sorted, and hence can be passed directly to the `submit_package` Bitcoin Core RPC. We avoid broadcasting non-TRUC parents-child packages via `submitpackage` for now to avoid adding a requirement to support `submitpackage` for users that don't enable zero-fee commitment channels. We will do so once support for `submitpackage` is more ubiquitous.
The electrsd dev-dependency does not yet ship binaries that support submitpackage on electrum, so we do not turn on 0FC channels in local tests, and instead only turn them on in a standalone CI workflow, which includes a custom electrs server that adds support for submitpackage on electrum. Co-Authored-By: HAL 9000
Anchor reserve accounting only looked at channels returned by the ChannelManager. That misses an important force-close transition: ChannelManager::list_channels can stop returning a channel before its commitment transaction confirms. During that window the ChannelMonitor can still broadcast, rebroadcast, or fee-bump the holder commitment and its anchor spend, so the wallet's emergency reserve must remain available. We now require a anchor reserve to be kept for non-trusted channels where the type is not yet known; we've recently made anchor channels always supported in ldk-node, so at this point it is safe to assume most channels of ldk-node will be anchor channels. We now no longer count existing 0FC channels to validate support for submitpackage on the chain source if those channels are in the trusted peers list. Co-Authored-By: HAL 9000
Store-backed integration tests drive wallet syncs explicitly. Leaving Esplora background sync enabled let the first background scan start before the funded address was tracked, so a later sync_wallets call could observe a zero premine balance. Reuse the manual Esplora sync config from shared test setup for Postgres, VSS, migration, and current-version compatibility builders. Co-Authored-By: HAL 9000
Prober::locked_msat only counted pending probe records from ChannelManager::list_recent_payments. Across restart, LDK can expose a still-locked outbound HTLC through channel state before the matching pending probe record is visible again, which made the restored locked budget read as zero. Fall back to unresolved outbound channel HTLCs when no recent pending probe accounting is available. Also make the restart test retry the probe-and-restart boundary if the probe failure wins the race before the node state is persisted. Co-Authored-By: HAL 9000
|
These last two commits here are one-shot attempts at fixing some CI issues I ran into, please take them with a grain of salt I have not audited them, let's see if we can get CI to pass here :) |
Hmm, so the Esplora fix makes somewhat sense to me, though not quite sure why it's suddenly needed everywhere. The second probing related commit should be unnecessary after #981? If not, we should definitely not introduce this commit here, as it changes completely unrelated behavior. Please drop the commit. |
tnull
left a comment
There was a problem hiding this comment.
Please squash.
As mentioned in above/comments, let's please split out the CI fixes and the reserve accounting changes to other PRs. Would also be good to revert all the non-0fc workflows/tests to their prior state as far as possible, just to keep this PR less invasive overall.
Other than that and some minor comments, we should be good to go.
| - name: Download bitcoind | ||
| if: "matrix.platform != 'windows-latest' && steps.cache-bitcoind.outputs.cache-hit != 'true'" | ||
| run: | | ||
| source ./scripts/download_bitcoind_electrs.sh |
There was a problem hiding this comment.
Now that we have the separate 0fc-integration.yml, can we revert all these changes in unrelated workflows to make this PR generally less invasive? Seems like we can just keep the old script and all workflows as-is, and just add a new script that's going to be used just by the 0fc tests workflow?
| waitForBlock(esploraEndpoint, lastBlockHash) | ||
| } | ||
|
|
||
| fun mineChannelClosureUntilSpendable(esploraEndpoint: String, nodes: List<Node>) { |
There was a problem hiding this comment.
I don't understand why we need all these Kotlin/Python changes?
| lsps2_service: Option<LSPS2Service>, | ||
| wallet: Arc<Wallet>, | ||
| channel_manager: Arc<ChannelManager>, | ||
| chain_monitor: Arc<ChainMonitor>, |
There was a problem hiding this comment.
Not sure I'm the biggest fan of wiring-through ChainMonitor everywhere. Maybe it would be better to have a dedicated reserve-tracker object for this rather than a util that requires an increasing number of parameters. It could live somewhere under src/channel (cf #946, where we'll probably add a store for channels under that path) or maybe balance.rs?
In any case, it seems like a more substantial change than expected, can we pull the reserve-calculation updates out to a separate (pre-factor or follow-up, your choice) PR?
| fn new(txs: &[(&Transaction, LdkTransactionType)]) -> Self { | ||
| Self(txs.iter().map(|(tx, tx_type)| ((*tx).clone(), Some(tx_type.clone()))).collect()) | ||
| let (txs, types) = | ||
| txs.iter().map(|(tx, tx_type)| ((*tx).clone(), Some(tx_type.clone()))).unzip(); |
There was a problem hiding this comment.
I think unzip also allocates, so not sure this change really saves us anything.
Given the two side-by-side Vecs are easier to misuse/get out of sync compared to a single Vec of tuples, maybe this might be worth reverting?
| /// the channel, which eliminates a very common source of channel force-closures. These | ||
| /// channels instead source *all* the fees required to confirm the commitment from the | ||
| /// anchor reserve of the channel closer at the time of force-close. If set, your chain | ||
| /// source *must* support the `submitpackage` Bitcoin Core RPC, and relay TRUC, P2A, and |
There was a problem hiding this comment.
nit: Can we link to resources for these? I.e., what's "TRUC", "P2A", and "ephemeral dust"?
| // before it exposes the matching pending probe payment again. ChannelDetails does | ||
| // not identify probe HTLCs, so fall back conservatively when no recent probe | ||
| // accounting is available. | ||
| self.channel_manager |
There was a problem hiding this comment.
This fallback seems brittle and may mask an upstream accounting or persistence-reconciliation issue? I think it may also undercount when one probe is reported but others are missing.
https://git.rust-bitcoin.org/lightningdevkit/rust-lightning/pulls/4806 is exposing a bit more info, that could be used to exclude at least the forwards in this fallback.
There was a problem hiding this comment.
Either way the fix doesn't belong in this PR. Rather, would be good to add over at #975 (maybe before checking whether the linked comments already address parts of it).
No description provided.